test: fix flaky tests by replacing hardcoded sleeps with polls - #263
Conversation
8fce899 to
1732c88
Compare
steve-chavez
left a comment
steve-chavez
left a comment
There was a problem hiding this comment.
Overall LGTM.
Formatted the code according to python standards.
Note: to make reviewing easier is better to do linting on a separate PR. It should also be enforced on CI.
|
The commits here are not looking that good for git history -- critical for when a bug arises on prod. Also a64f20d is labeled as It looks to me all of these can be squashed onto 3 commits:
@imor Before merging, could you do that? 🙏 Edit: Non-blocking request, but would be good to have. Feel free to merge if you disagree. |
No worries, will rewrite into a cleaner history @steve-chavez. I know this was not the easiest of PRs to review so thanks for you time 🙏🏼 |
Instead of using sleeps in tests which assumed that certain operations will complete within those timeouts, we now poll for events to ensure that tests pass deterministically everywhere: locally and in ci. Before this change the tests would pass locally but will fail in ci. Such flaky tests make it hard to make changes in the code because the confidence in the tests went down and developers could dismiss genuine failures due to alert fatigue.
We no longer download all PG versions to reduce CI time. xpg 2.5.0 added support for downloading only one PG version. We bumped to xpg 2.5.0 to use this feature and updated commands in CI to use this feature. We also avoid running two workflows on push to a PR branch. The on: [push, pull_request] ran two duplicate workflows for each push to a PR branch. This was wasteful. Now we only run one workflow for each push to a PR branch. We also run a workflow once commits land on the master branch, e.g. after a PR is merged.
ca6b6ec to
fcb0717
Compare
2 participants
This PR started off as an attempt to fix flaky tests but has grown into a mini overhaul of the tests. The changes include:
time.sleep(...)calls and race conditions and assumptions in the test code. The improvement is evidenced in the last 12 or so commits in this PR. Before this PR, the tests would fail quite often. Examples: one, two, three, four, and many more. The new code waits for certain events to happen instead of hardcoded timeouts which is more robust.common.pywhich also makes test smaller and easier to read.CONTRIBUTING.mdto show how to run a single or a subset of tests.masterstill triggers a separate test run.Further improvement planned in a separate PR: currently test on each PG version unnecessarily pulls in all PG versions in nix store via xpg. This is ~700 MB worth of data pulled from the cache. This can be reduced if a single test could only copy that specific version of PG.